Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of test-reliability issues to address (builtin-extensions step timeout still set to 120s at the TestStep level, and remove_all_finalizers uses a fragile JSONPatch replace instead of the repo’s more robust merge-patch pattern).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the E2E (KUTTL) suite for the PostgreSQL operator to improve upgrade/extension coverage and reduce test flakiness, including switching the operator-upgrade-extensions scenario to pg_stat_statements and tightening test infrastructure behaviors (GKE version selection, log filtering, cleanup finalizers, and timeouts).
Changes:
- Switch operator-upgrade-extensions E2E to use
pg_stat_statementsinstead ofpg_stat_monitor. - Improve test infra robustness: select only STABLE GKE versions (one patch per minor) and filter operator error logs to the active KUTTL namespace.
- Adjust cleanup/finalizer handling and deployment timeouts to reduce upgrade/migration test flakes.
File summaries
| File | Description |
|---|---|
| e2e-tests/tests/operator-upgrade-extensions/conf/operator-upgrade-extensions.yaml | Enables pg_stat_statements as a builtin extension for the upgrade test cluster. |
| e2e-tests/tests/operator-upgrade-extensions/03-enable-extensions-and-write-data.yaml | Creates/extensions workload using pg_stat_statements during the upgrade scenario. |
| e2e-tests/tests/operator-upgrade-extensions/01-create-cluster.yaml | Validates builtin extension fields weren’t pruned by the released CRD (now checks pg_stat_statements). |
| e2e-tests/tests/migration-from-crunchy-standby/99-cleanup.yaml | Uses replace to clear finalizers during cleanup. |
| e2e-tests/tests/migration-from-crunchy-pv/99-cleanup.yaml | Uses replace to clear finalizers during cleanup. |
| e2e-tests/tests/migration-from-crunchy-pv/03-migrate-pv.yaml | Uses replace to clear finalizers before deleting Crunchy source cluster. |
| e2e-tests/tests/migration-from-crunchy-backup-restore/99-cleanup.yaml | Uses replace to clear finalizers during cleanup. |
| e2e-tests/tests/builtin-extensions/00-deploy-operator.yaml | Moves/increases deployment timeout handling for the builtin-extensions test step. |
| e2e-tests/functions | Updates helper functions for finalizer removal, GKE version selection, and operator log error filtering. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -1,7 +1,8 @@ | |||
| apiVersion: kuttl.dev/v1beta1 | |||
| kind: TestStep | |||
| commands: | |||
There was a problem hiding this comment.
this is incorrect, timeout for TestStep must be in script not in the class kuttl doc
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new operator log namespace filter only handles JSON-style namespace fields and doesn’t reliably filter console-formatted logs as intended, which can cause unrelated namespace errors to fail tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
e2e-tests/functions:2645
-
- Problem: The new namespace filter in
check_logs_for_errorsonly recognizes JSON fields like"namespace":"...", but the comment above says operator logs are in console encoder format whenLOG_STRUCTURED=false.
- Problem: The new namespace filter in
- Why it matters: Errors from other KUTTL namespaces can still pass the filter in console-formatted logs, causing unrelated failures and defeating the intent of scoping operator errors to the current test namespace.
- Fix: Extend the
awkfilter to also detect and filter common console field formats (e.g.,namespace=<ns>/namespace="<ns>"), while still allowing lines with no namespace.
| awk -v namespace="$NAMESPACE" '
!match($0, /"namespace"[[:space:]]*:[[:space:]]*"[^"]+"/) ||
index($0, "\"namespace\":\"" namespace "\"") ||
index($0, "\"namespace\": \"" namespace "\"")
' \
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
commit: 0a0db5e |
…fixes (#1808) * Increase deployment time that includes minio * Use only STABLE version on GKE * Filter logs by test namespace to avoid getting other namespaces error logs * Use pg_stat_statements in operator-upgrade-extensions * Replace finalizers instead of removing to avoid conflicts * Fix pg_stat_statements for test asserts * Patch resource instead of replacing it Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
CHANGE DESCRIPTION
Description:
Use pg_stat_statements instead of pg_stat_monitor in the operator-upgrade-extensions test
Additionally:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability